Configure Copilot Code Review for the provider repo#3425
Configure Copilot Code Review for the provider repo#3425robert-crandall wants to merge 18 commits into
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
There was a problem hiding this comment.
Pull request overview
Adds repository-wide and path-scoped Copilot Code Review (CCR) instruction files to align automated review feedback with this provider’s conventions (notably Go 1.24 toolchain assumptions and the intentional “no read-after-write” pattern from #2892), reducing low-value churn on PRs.
Changes:
- Added repo-wide CCR guidance in
.github/copilot-instructions.md(toolchain anchors, severity policy, provider-specific review priorities). - Added path-scoped CCR instruction files for provider Go code, tests, examples, and website docs under
.github/instructions/.
Show a summary per file
| File | Description |
|---|---|
| .github/copilot-instructions.md | Defines repo-wide CCR policy (Go/tooling context, severity rules, provider-specific review checklist). |
| .github/instructions/schema-and-state.instructions.md | Path-scoped guidance for github/**/*.go focusing on schema/state compatibility, CRUD behavior, and API safety. |
| .github/instructions/tests.instructions.md | Path-scoped guidance for github/**/*_test.go covering expectations for test updates and acceptance testing. |
| .github/instructions/examples.instructions.md | Path-scoped guidance for examples/** enforcing example structure, provider config rules, and sensitive handling. |
| .github/instructions/docs.instructions.md | Path-scoped guidance for website/** ensuring docs stay in sync with schema, imports, and permissions. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
deiga
left a comment
There was a problem hiding this comment.
This is looking really good!
- Correct Go version anchor: 1.24 -> 1.26 (matches go.mod). - Drop ValidateFunc everywhere; ValidateDiagFunc only (ValidateFunc is deprecated and not allowed in this repo). - Rewrite docs.instructions.md to target templates/** instead of the no-longer-existing website/**. Docs under docs/** are auto-generated; edits go in templates/, examples/, or resource Description fields. - Replace remaining website/ references in copilot-instructions.md with docs/ and templates/. - Add repository-rename convention to schema-and-state.instructions.md: attribute must be 'repository' (not ForceNew), plus computed 'repository_id', plus CustomizeDiff: diffRepository. - Add Delete 404 handling: treat as success, not error. - Require *Context CRUD variants and diag.Diagnostics on all new resources. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review @deiga! Addressed all 11 comments in 96bacb6:
Let me know if any of the framing on the repository-rename pattern needs tweaking; that one was new to me and I leaned on |
|
The new changes cover pretty much everything I noticed! Thanks @robert-crandall I just remembered that it could be useful to include mention that we now use
|
Add a 'terraform-plugin-testing Conventions' section to .github/instructions/tests.instructions.md covering: - Prefer ConfigStateChecks over the legacy Check / ComposeTestCheckFunc pattern. - Use ValueComparers (compare.ValuesSame / compare.ValuesDiffer) for cross-step value comparisons instead of custom pointer structs. - Don't flag legacy patterns in unmodified tests; only in new or substantially modified ones. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good call! Added a |
|
@robert-crandall how about https://awesome-copilot.github.com/instructions/#file=instructions%2Fgo.instructions.md for the Go code? I have some customizations to suggest op top of that but it's a good starting point for generic Go code. |
Incorporates the upstream Go instructions from https://github.com/github/awesome-copilot/blob/main/instructions/go.instructions.md as a path-scoped instructions file (applyTo: **/*.go,**/go.mod,**/go.sum). A preamble subordinates the Go guidance to the repo's existing severity policy (HIGH/MEDIUM only, no LOW or nits) and notes the Go-version anchors in copilot-instructions.md plus the no-read-after-write exception, so CCR doesn't suddenly start surfacing style nits or fight provider-specific conventions. The main copilot-instructions.md now lists every path-scoped file so the set is discoverable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good idea! Done in 73cda9b |
stevehipwell
left a comment
There was a problem hiding this comment.
Just a couple of standard changes to the go based instructions.
Co-authored-by: Steve Hipwell <steve.hipwell@gmail.com>
|
@robert-crandall have you seen/run the Chat Customizations Evaluations VS Code extension? |
Path-specific instruction files now cross-link to copilot-instructions.md
(and to go.instructions.md from the Go-scoped files), with several
content fixes on top.
docs.instructions.md
- Add a 'How Doc Generation Works' section explaining tfplugindocs,
the templates/examples/schema layout, and the default-template
fallback.
- Add a 'Local Doc Workflow' section calling out make generatedocs,
make validatedocs, make checkdocs, and the rumdl targets.
- Rewrite the schema-sync section as 'fix forward' guidance: fix
descriptions at the schema or template source, not in the generated
docs/.
- Soften the Imports section: tfplugindocs auto-generates the import
section from examples/resources/<name>/import.sh, so reviewers just
need to verify that section renders.
- Recommend linking to the GitHub REST API reference for permission
and scope info to avoid stale documentation.
- Note that terraform fmt is the formatting baseline for example HCL.
- Add a relative-link example for internal doc cross-references.
examples.instructions.md
- Distinguish the two kinds of example: single-file tfplugindocs
snippets (under examples/{provider,resources,data-sources}/) and
root-module examples (everything else). Standard-module-structure
rules now apply only to root-module examples.
- Call out that tfplugindocs snippets must not declare
required_providers or a provider block.
schema-and-state.instructions.md, tests.instructions.md
- Add cross-references to go.instructions.md.
schema-and-state.instructions.md
- Replace the ListOptions/NextPage pagination guidance with a
recommendation to use the *Iter methods on google/go-github that
return iter.Seq2[T, error]. Hand-rolled NextPage loops in existing
code are still acceptable, but flag new code that re-implements
pagination when an *Iter method exists.
go.instructions.md
- Add a short intro paragraph above the existing callout that links
to copilot-instructions.md and the other path-scoped files.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
go.instructions.md
- Switch the applyTo glob from comma-separated to brace expansion
('**/{*.go,go.mod,go.sum}'). Brace expansion is universally
minimatch-compatible, so this removes any doubt about whether the
Go instructions actually apply to .go / go.mod / go.sum files.
copilot-instructions.md
- Resolve the conflict between 'ALWAYS acknowledge instructions are
being used' and 'must say only No blocking findings found and stop'.
Reviews with findings open with a dedicated acknowledgment sentence;
reviews with no findings collapse the two requirements into one
verbatim sentence:
'These provider review instructions are being used. No blocking findings found.'
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CCR pointed out that docs.instructions.md flags manual edits to docs/**
as HIGH but its front matter only applies to templates/**, so the rule
won't fire on PRs that actually touch the generated docs.
- docs.instructions.md: applyTo now '{templates,docs}/**'; opening
paragraph updated to match.
- copilot-instructions.md: bullet updated so the instruction map
reflects the new coverage.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CCR caught two factual errors in the previous round: - examples/provider/provider.tf does not exist in this repo. The provider landing page (templates/index.md.tmpl) embeds examples/example_*.tf from the root of examples/ instead. - The flat 'no required_providers / no provider block in tfplugindocs snippets' rule contradicts the landing-page snippets, which intentionally include both because the landing page is the first thing users see. docs.instructions.md - Updated the 'Example HCL under examples/' list in 'How Doc Generation Works' to reflect the real layout: example_*.tf at the root for the landing page (with required_providers + provider), and per-resource / per-data-source snippets under examples/resources/<name>/ and examples/data-sources/<name>/ without those blocks. examples.instructions.md - Same correction in 'Two Kinds of Examples'. - Provider Configuration bullet now scopes the 'no required_providers / no provider block' rule to per-resource and per-data-source snippets, with an explicit carve-out for the examples/example_*.tf landing-page snippets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CCR pointed out that schema-and-state.instructions.md applies to 'github/**/*.go', which also matches '_test.go' files, creating an overlap with tests.instructions.md. Minimatch does not support negation globs without an explicit 'negate: true' option (the '!' prefix is ignored by default), so I can't reliably narrow the applyTo to exclude '_test.go'. Instead I spell out the precedence in the file itself: on test files, tests.instructions.md is authoritative and the schema / CRUD / API-safety bullets here are background, to be flagged only when the test actually touches that code (e.g., hand-rolled pagination in an acceptance test). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| 1. **`tfplugindocs` examples** - the single-file snippets that get | ||
| embedded into the auto-generated docs. These live under: | ||
|
|
||
| - `examples/example_*.tf` - root-level snippets used by the provider |
There was a problem hiding this comment.
| - `examples/example_*.tf` - root-level snippets used by the provider | |
| - `examples/provider/**/*.tf` - root-level snippets used by the provider |
The path is being changed in #3196.
Note that files using the pattern examples/provider/provider*.tf can be automated via the .Examples templating capabilities.
| - `examples/resources/<resource_name>/example_N.tf` - per-resource | ||
| snippets (plus optional `import.sh` for the import section). | ||
| - `examples/data-sources/<data_source_name>/example_N.tf` - | ||
| per-data-source snippets. |
There was a problem hiding this comment.
This is the pattern we currently use, but it is not how we want to work by default. By default examples should be a single file and any additional context should be in a comment at the top. The file name format is examples/resources/<resource_name>/resource*.tf & examples/data-sources/<data_source_name>/data-source*.tf. The current pattern using example*.tf should only be used where we're manually calling for the template and as such should be a single file with any additional context directly placed into the template.
This PR adds Copilot instructions to this repo.
Before the change?
.github/copilot-instructions.mdand no path-scoped.github/instructions/*.mdfiles, so Copilot Code Review (CCR) ran with no project-specific guidance. That meant a lot of churn on PRs: low-value nits, suggestions based on older Go toolchains, and missing context about provider-specific conventions (notably the intentional "no read-after-write" pattern, see [MAINT]: Remove read call from create/update #2892).After the change?
.github/copilot-instructions.mdwith the repo-wide review policy:go.mod) so CCR doesn't suggest pre-1.22 loop-variable shadowing, rewriterange over int, or flag generics /min/max/clear/slices/maps/iter.Seq/iter.Seq2/WaitGroup.Goas unknown.Sensitive: trueon secret-bearing attributes, schemaDescription,ValidateDiagFuncon bounded inputs (the deprecatedValidateFuncis explicitly not allowed), examples for new resources, docs for schema changes, import ID format..github/instructions/:schema-and-state.instructions.md- applies togithub/**/*.go; covers schema/state compatibility, CRUD behavior, API safety (including the*Iterpagination pattern fromgoogle/go-github).tests.instructions.md- applies togithub/**/*_test.go; coverage expectations andterraform-plugin-testingconventions (ConfigStateChecks,ValueComparers).examples.instructions.md- applies toexamples/**; distinguishes the two kinds of example (single-filetfplugindocssnippets vs root-module examples), provider-block rules, sensitive variable handling.docs.instructions.md- applies to{templates,docs}/**; howtfplugindocsgeneration works, keeping docs in sync with the schema (fix forward), import section, permission/scope links to the REST API, relative-link convention.go.instructions.md- idiomatic Go reference scoped to**/{*.go,go.mod,go.sum}(subordinate to the repo-wide severity policy and to the path-scoped files above).I think this should noticeably cut the contributor friction CCR has been generating, especially around Go-version false positives and stylistic nits. If something turns out to be too restrictive (or not restrictive enough), the instruction files are easy to iterate on.
Pull request checklist
Docs/tests checkboxes left unchecked: this PR only touches
.github/config for CCR. No provider code, no schema, no user-facing docs are affected.Does this introduce a breaking change?